ci: stop moot merge-queue runs from ejecting healthy PRs from CodeQL - #3633
Merged
Merged
Conversation
A merge-queue batch can dissolve while the CodeQL job is still running. GitHub deletes the ephemeral gh-readonly-queue ref, and the SARIF upload -- which begins ~10 minutes in, after the analysis has already succeeded -- fails with "ref ... not found in this repository". Nothing is being merged at that point, so the run is moot, but the queue reads the red check as the PR's fault and ejects it. Observed four times on 2026-08-12 (#3626 twice, #3624, #3606). On run 31573922804 the sibling CI/CD run for the identical head SHA passed, and the PR was a pure file deletion that cannot produce a security finding. Capture the analyze step's outcome and re-raise it unless the target ref has provably gone away, or moved to a different SHA because the batch was rebuilt without this run. Everything else -- any non-merge_group event, a ref still live at our SHA, or an indeterminate API response -- still fails the build, so a genuine finding is unaffected.
📝 WalkthroughWalkthroughThe CodeQL workflow now limits analysis to 25 minutes. It conditionally tolerates failures for merge-group runs when the target ref is missing or moved, while failing closed for other conditions and recording tolerated failures. ChangesCodeQL failure handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Audit follow-up on the moot merge-queue guard. Six fixes, no restructuring.
The 200 branch read the ref SHA with `jq -r '.object.sha // empty'` and
compared it to ours. A 200 carrying valid JSON without an object SHA left
the variable empty, which compares unequal and fell straight into the
tolerant "batch was rebuilt without us" path -- exit 0, failure swallowed.
Executing the old script against bodies `{}`, `{"object":null}`,
`{"object":{}}` and an empty body reproduces it: all exit 0 and print the
self-evidently broken `has moved from <sha> to :`. An unparseable body was
no better, aborting on jq's exit 5 with no diagnostic. An absent SHA now
fails closed and says the response was unreadable.
curl already emits 000 through -w on a transport failure, so the
`|| echo "000"` fallback appended a second sentinel: the status became the
literal `000000`, and the operator-facing message read
`(HTTP 000000)`. Measured with curl 8.7.1 for connection refused, DNS
failure and timeout. Classification was unaffected -- it still fell to the
catch-all and failed closed -- but the message was wrong. Removed.
The request had no timeout and no retry, and the job had no
timeout-minutes. A hang against api.github.com would have held the job for
the Actions default of 360 minutes, far past the queue's 1800s
check_response_timeout, reproducing the very ejection this guard prevents.
Bounded to ~2 minutes worst case and capped the job at 25.
The guard's safety rests on the queue's merge_method being SQUASH, which
mints a fresh SHA per rebuild so a moot run's SHA cannot recur on a live
entry. Under REBASE a no-op rebase could repeat a SHA and let a moot green
satisfy a later live entry. Not reachable today, but rebaseMergeAllowed is
true on this repo, so the assumption is now recorded where it can be seen.
The step comment claimed only a vanished ref is tolerated. It actually
swallows any analyze failure coinciding with dissolution, an extractor
crash included. Safe for the same SHA-binding reason, but now stated.
Tolerated paths append to GITHUB_STEP_SUMMARY, since turning a red required
check green was previously visible only in the raw log. The mktemp body is
removed on exit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/codeql.yml (1)
102-114: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider passing the token through stdin instead of argv.
-H "Authorization: Bearer ${GH_TOKEN}"places the token in the process arguments. Any process on the runner can read it from the process table. The runner is ephemeral and the token is scoped to the run, so the exposure is small. Reading the header from stdin removes it.The retry and timeout bounds themselves look correct.
curlruns without-f, so 4xx and 5xx responses still return exit code 0 and real status codes, and--retry-all-errorsdoes not retry the 404 the guard depends on.♻️ Proposed refactor to keep the token off argv
- if ! status="$(curl -sS \ + if ! status="$(printf 'header = "Authorization: Bearer %s"\n' "$GH_TOKEN" | curl -sS \ + --config - \ --connect-timeout 10 \ --max-time 30 \ --retry 3 \ --retry-delay 2 \ --retry-all-errors \ -o "$body" -w '%{http_code}' \ - -H "Authorization: Bearer ${GH_TOKEN}" \ -H "Accept: application/vnd.github+json" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/codeql.yml around lines 102 - 114, Update the curl invocation in the CodeQL workflow to supply the GitHub authorization header through stdin rather than embedding GH_TOKEN in the command-line arguments, while preserving the existing URL, status capture, retry, timeout, and error-handling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codeql.yml:
- Around line 136-148: Validate current after parsing in the HTTP 200 branch,
requiring exactly one 40-character hexadecimal SHA before comparing it with
TARGET_SHA. Treat empty, multi-line, or otherwise malformed values as
undetermined and retain the existing failing-closed error path; only valid SHAs
may reach the rebuilt-batch comparison.
---
Nitpick comments:
In @.github/workflows/codeql.yml:
- Around line 102-114: Update the curl invocation in the CodeQL workflow to
supply the GitHub authorization header through stdin rather than embedding
GH_TOKEN in the command-line arguments, while preserving the existing URL,
status capture, retry, timeout, and error-handling behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3335f56d-7ad2-4a7d-a071-3f314bd59bef
📒 Files selected for processing (1)
.github/workflows/codeql.yml
Comment on lines
+136
to
+148
| if ! current="$(jq -r '.object.sha // empty' "$body")"; then | ||
| current="" | ||
| fi | ||
| if [ -z "$current" ]; then | ||
| echo "::error::${TARGET_REF} returned HTTP 200 but the response carried no object SHA," \ | ||
| "so whether this run is still live cannot be determined. Failing closed." | ||
| exit 1 | ||
| fi | ||
| if [ "$current" = "$TARGET_SHA" ]; then | ||
| echo "::error::${TARGET_REF} still points at ${TARGET_SHA}, so this run is live." \ | ||
| "The CodeQL failure is genuine. Failing the build." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the shape of current before the moot branch.
The 200 branch fails closed on an empty or unparseable SHA. It does not check that current is a single SHA. jq -r '.object.sha // empty' emits one line per JSON document in $body. If $body ever holds more than one document, current becomes a multi-line string. That string is non-empty and unequal to TARGET_SHA, so control reaches the tolerant "batch was rebuilt" path and reports a failing run as successful. Restricting current to one 40-hex value keeps the branch closed for every malformed body.
🛡️ Proposed fix to constrain the parsed SHA
- if ! current="$(jq -r '.object.sha // empty' "$body")"; then
+ if ! current="$(jq -er 'if type == "object" then (.object.sha // empty) else empty end' "$body")"; then
current=""
fi
- if [ -z "$current" ]; then
+ case "$current" in
+ *[!0-9a-f]* | "") current="" ;;
+ esac
+ if [ -z "$current" ] || [ "${`#current`}" -ne 40 ]; then
echo "::error::${TARGET_REF} returned HTTP 200 but the response carried no object SHA," \
"so whether this run is still live cannot be determined. Failing closed."
exit 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ! current="$(jq -r '.object.sha // empty' "$body")"; then | |
| current="" | |
| fi | |
| if [ -z "$current" ]; then | |
| echo "::error::${TARGET_REF} returned HTTP 200 but the response carried no object SHA," \ | |
| "so whether this run is still live cannot be determined. Failing closed." | |
| exit 1 | |
| fi | |
| if [ "$current" = "$TARGET_SHA" ]; then | |
| echo "::error::${TARGET_REF} still points at ${TARGET_SHA}, so this run is live." \ | |
| "The CodeQL failure is genuine. Failing the build." | |
| exit 1 | |
| fi | |
| if ! current="$(jq -er 'if type == "object" then (.object.sha // empty) else empty end' "$body")"; then | |
| current="" | |
| fi | |
| case "$current" in | |
| *[!0-9a-f]* | "") current="" ;; | |
| esac | |
| if [ -z "$current" ] || [ "${#current}" -ne 40 ]; then | |
| echo "::error::${TARGET_REF} returned HTTP 200 but the response carried no object SHA," \ | |
| "so whether this run is still live cannot be determined. Failing closed." | |
| exit 1 | |
| fi | |
| if [ "$current" = "$TARGET_SHA" ]; then | |
| echo "::error::${TARGET_REF} still points at ${TARGET_SHA}, so this run is live." \ | |
| "The CodeQL failure is genuine. Failing the build." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/codeql.yml around lines 136 - 148, Validate current after
parsing in the HTTP 200 branch, requiring exactly one 40-character hexadecimal
SHA before comparing it with TARGET_SHA. Treat empty, multi-line, or otherwise
malformed values as undetermined and retain the existing failing-closed error
path; only valid SHAs may reach the rebuilt-batch comparison.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
The merge queue ejects healthy PRs because CodeQL reports a failure for a run that is no longer attached to anything.
Mechanism, verified
A merge-queue batch dissolves while the CodeQL job is still running. GitHub deletes the ephemeral
gh-readonly-queue/**branch, and the SARIF upload — which only starts about ten minutes in, after the analysis itself has already succeeded — 404s against the ref that no longer exists. The queue reads that red check as the PR's fault and ejects it.Evidence from run 31573922804 (CodeQL,
merge_group, pr-3626):added_to_merge_queue570f612fpr-3624on base2b3a0e096— the pr-3626 batch is goneUploading resultsref ... not found in this repositoryremoved_from_merge_queueThree things this pins down:
Exported results to SARIF (524ms)andCodeQL scanned 5702 out of 5789 TypeScript files. The only thing that failed is the upload.Analyzewas the only failing check of 28 on570f612f, and docs: remove stale DISTRIBUTION.md #3626 is a pure 621-line file deletion.Confirmed the deleted ref really is gone, and that a live ref answers normally — this is what the guard keys off:
How often this actually bites
An earlier draft of this description claimed four healthy runs were ejected today. That number was inflated: it counted every run carrying the ref-404 signature, without checking whether the run had other failures that would have ejected it anyway. Corrected against the check-run data for each SHA:
349c4ec7tests (bun)aa4ee6cbtests (unit),coverage gate,coverage shard 4/8570f612f046ed751ff57ccf2tests (binary e2e)All five carry the identical
ref ... not found in this repositorysignature, so the bug is real and recurrent. But only two of them —570f612f(#3626) and046ed751(#3624) — were otherwise entirely green, and those are the only ejections this change would have prevented. The other three had genuine failures and belonged out of the queue.e0cc27da, an adjacent attempt on the same queue branch, is not in the table at all: its CodeQL run succeeded, and it was ejected fortests (unit),coverage gateandcoverage shard 4/8.Two per day is still worth fixing — CodeQL takes ~10 minutes while CI/CD takes ~6, so CodeQL is structurally the job still in flight when a batch dissolves, and the rate scales with queue churn.
What
Analyzeactually gatesWorth stating plainly, because it bounds how much this change can cost.
Analyzeis a required status check onmain:What it gates is "did the analysis run and upload successfully" — not "are there findings". On this repo:
codeql-action/analyze@v4.37.6has no fail-on-findings input. Its inputs areadd-snippets, category, check_name, checkout_path, cleanup-level, expect-error, matrix, output, post-processed-sarif-path, ram, ref, sha, skip-queries, threads, token, upload, upload-database, wait-for-processing— nothing that fails the job on a result.code-scanning/default-setupreportsstate: not-configured).js/incomplete-multi-character-sanitization, high) has been open since 2026-08-11T19:19Z, and 20+ PRs have merged tomainsince. Four alerts are open in total.So this guard cannot suppress a finding-based block, because no such block exists here. It affects only whether a run that produced no usable upload is allowed to fail the queue.
On removing the
merge_grouptriggerThe reasoning holds — do not remove it. The merge queue waits for exactly the required contexts to report on the
merge_groupref. Drop the trigger and the check never arrives; the queue would sit untilcheck_response_timeout(1800s) expires and then eject the PR anyway — the same symptom, six times slower. The comment in the workflow is correct and stays.Why not
continue-on-erroron the jobA blanket
continue-on-error: trueonanalyzewould make every CodeQL failure non-blocking on every event, includingpushandschedule, where a broken analysis means the repo silently stops being scanned. Not done.The fix
There is no supported upstream mechanism. github/codeql-action#1572 is this exact error, open since March 2023; a maintainer said they were "discussing internally how best to support merge queue" and nothing shipped. The
analyzeaction's inputs offer no way to say "this run is moot". So the guard is hand-rolled, deliberately narrow, and fails closed.The
analyzestep's outcome is captured rather than failing the job outright, and a guard step re-raises it unless the target ref has provably gone away:merge_group, ref returns 404 — deletedmerge_group, ref returns 200 but SHA ≠ ours — batch rebuilt without usmerge_group, ref returns 200 at our SHA — run is livemerge_group, ref returns 200 with no readable SHAmerge_group, any other HTTP status or transport errorpull_request/push/scheduleWhat the tolerant path really swallows
Stated honestly, because the first draft of the workflow comment overstated it: the guard tolerates any analyze failure on a
merge_grouprun whose ref has vanished or moved — not only the upload 404. An extractor crash or a query failure landing in the same window is swallowed too.That is safe for one reason: check runs are bound to a SHA. If the queue branch is gone or has moved, nothing can be merged on the strength of this run, and the rebuilt entry re-runs CodeQL from scratch on a new SHA. A moot pass can never let unreviewed code through; it only stops the queue attributing a dead run to a live PR.
This rests on the queue's
merge_methodbeingSQUASH, which mints a fresh SHA for every rebuild, so a moot run's SHA can never recur on a later live entry. UnderREBASE, a no-op rebase could reproduce the same SHA and let a moot green satisfy a live entry.rebaseMergeAllowedistrueon this repo, so the queue's method is the only thing closing that hole — the assumption is now recorded in the workflow so a future config change does not silently reopen it.Verification
The guard script is extracted from the committed YAML and executed by bash against a real local HTTP server and real transport failures, so the actual
curlinvocation — flags, retries,-wsentinel — is under test rather than a re-implementation.tmpasserts themktempbody was removed;sumasserts a$GITHUB_STEP_SUMMARYrecord exists on exactly the tolerated paths. Every case also asserts the transport sentinel is three zeroes, never000000.Running the same suite against the previous revision of this branch fails 10 of 16, which is what makes the suite worth anything:
Those
exit 0s are a real fail-open, and they printed:An empty SHA compared unequal to ours and fell into the tolerant path. It now fails closed and says the response was unreadable. The unparseable-JSON case exited 5 —
jqaborting underset -ewith no diagnostic at all.shellcheck 0.11.0is clean on the extracted script.Trade-off
This is a workaround for a GitHub-side interaction, and it owns two risks:
I considered
upload: neverformerge_group, which is a documented input and would remove the 404 deterministically. Rejected: it drops the SARIF upload on every queue run, not just the broken ones, and would silently break code-scanning merge protection if it is ever enabled here.Note
This PR has to survive the very queue it is fixing, on the old workflow — the fix only takes effect for runs after it lands. If its own CodeQL check goes red with the ref-not-found signature, that is the bug reproducing itself, not a reason to doubt the change.
🤖 Generated with Claude Code
Summary by CodeRabbit